Skip to content

BooleanQuery rewrite for must_not RangeQuery clauses #17655

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 14 commits into from
Jun 4, 2025
Merged

BooleanQuery rewrite for must_not RangeQuery clauses #17655

merged 14 commits into from
Jun 4, 2025

Conversation

peteralfonsi
Copy link
Contributor

@peteralfonsi peteralfonsi commented Mar 21, 2025 •
edited
Loading

Description

This PR automatically rewrites boolean queries which have a must_not RangeQuery clause to instead use a should clause of the complement of that range. This can be 2-30x faster depending on the query. See #17586 where this is described in more detail.

Example original query (on nyc_taxis):

"bool" : { 
  "must_not": [ 
    {"range" : {"dropoff_datetime" : {"gte": "01/07/2015", "lte": "01/09/2015", "format": "dd/MM/yyyy"}}}
  ]
}

Rewritten query:

"bool": { 
  "must":{
    "bool":{
      "should": [
        {"range" : {"dropoff_datetime" : {"lt": "01/07/2015", "format": "dd/MM/yyyy"}}},
        {"range" : {"dropoff_datetime" : {"gt": "01/09/2015", "format": "dd/MM/yyyy"}}}
      ]
    }
  }
}

Some benchmark numbers from http_logs and nyc_taxis (excluded ranges are on @timestamp and dropoff_datetime fields respectively). "Originally written as" means whether the query was sent to OpenSearch with a must_not clause, or if it was sent already rewritten with should clauses. Ideally, after the changes are applied, these p50s should be the same.

Excluded range Originally written as Dataset p50 before changes (ms) p50 after changes (ms)
6/10 - 6/13 must_not http_logs 259 38.2
6/10 - 6/13 should http_logs 34.2 39.5
6/9 - 6/10 must_not http_logs 269 30.8
6/9 - 6/10 should http_logs 26.3 30.8
7/1 - 9/1 must_not nyc_taxis 873 408
7/1 - 9/1 should nyc_taxis 427 405
1/1 - 9/1 must_not nyc_taxis 1214 111
1/1 - 9/1 should nyc_taxis 116 111
1/1 12:00 - 1/1 12:01 must_not nyc_taxis 599 19.5
1/1 12:00 - 1/1 12:01 should nyc_taxis 19.3 20.2

I believe the small differences between runs (for example, 7/1-9/1 should going from 427 -> 405 ms, when we'd expect no change) is just due to variation between different runs/instances. This is expected from what I've seen in tiered caching benchmarks. I've done a few runs and the direction/magnitude of the changes vary.

Related Issues

Part of #17586

Check List

  • Functionality includes testing.
  • [N/A] API changes companion pull request created, if applicable.
  • [N/A] Public documentation issue/PR created, if applicable.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Peter Alfonsi added 2 commits March 20, 2025 09:55
@peteralfonsi peteralfonsi marked this pull request as draft March 21, 2025 20:48
Peter Alfonsi added 2 commits March 21, 2025 13:51
@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for d9eee10: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Fix forbiddenApis
25367bb 
Signed-off-by: Peter Alfonsi <[email protected]>
@github-actions GitHub Actions
Copy link
Contributor

✅ Gradle check result for 25367bb: SUCCESS

@msfroh
Copy link
Contributor

msfroh commented Mar 26, 2025

@peteralfonsi -- can you write + run some tests when docs have more or fewer than 1 value for the field being queried?

I think this optimization makes sense when each doc has exactly 1 value for the field, but I think we'll run into problems in the following cases:

  1. If a doc doesn't have the field at all, it would be returned by the MUST_NOT query, but won't be returned by the disjunction.
  2. If a document has 2 values, one in the range and one outside, it would not be returned by the MUST_NOT query, but will be returned by the disjunction.

The good news is that (at the shard level), we can cheaply check for the "exactly one value" case. There's Lucene code that does something similar here.

@peteralfonsi
Copy link
Contributor Author

@msfroh This is a good point I didn't consider.

For the missing values case, we can also add a third should clause which is just "must_not":{"exists":{"field":<field_name>}}. Then documents missing values for that field will be returned as they were before the rewrite. I tested this quickly on nyc_taxis and I didn't see any slowdown. However I still need to try it on a modified nyc_taxis where we actually have documents with missing values, so TBD on the latencies for that.

The >1 value case is trickier. PointRangeQuery can check if any docs have >1 value through the LeafReader. But I don't think we can get LeafReader at query rewrite time. A Query is rewritten during SearchService.createContext() before it ever produces a Weight in SearchService.loadOrExecuteQueryPhase(). Then the Weight only accesses a LeafReaderContext once it runs scorer(leafReaderContext). QueryRewriteContext or even DefaultSearchContext don't have access to individual LeafReaders, right?

Would it be crazy for OpenSearch itself to keep track of which fields have documents with >1 value, on a per-shard basis? This could be updated at indexing/refresh time maybe. I think it could be worthwhile for more than just this rewrite. It would at least apply to 2 of the other BooleanQuery rewrites I'm planning.

Alternatively are there any other ways to check this that already exist?

Peter Alfonsi added 4 commits April 16, 2025 14:59
@peteralfonsi
Copy link
Contributor Author

Sorry for the very long delay, was caught up with other things. Looks like the method described by @froh is actually doable at QueryBuilder rewrite time, since we have access to LeafReaderContexts via IndexSearcher via QueryShardContext. I've added this check. I also reran the benchmarks to make sure getting PointValues wasn't slowing things down, but there was no change in latencies.

@github-actions GitHub Actions
Copy link
Contributor

❕ Gradle check result for fc49ddd: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@opensearch-ci-bot opensearch-ci-bot mentioned this pull request Apr 18, 2025
Open
@opensearch-trigger-bot
Copy link
Contributor

This PR is stalled because it has been open for 30 days with no activity.

@opensearch-trigger-bot opensearch-trigger-bot bot added the stalled Issues that have stalled label May 18, 2025
msfroh
msfroh reviewed May 19, 2025
View reviewed changes
Copy link
Contributor

@msfroh msfroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the delay in getting back to this one. I accidentally lost track of it.

The additional changes look safer, thanks! I just had a small clean-up suggestion on getting the LeafReaderContexts.

Address LeafReaderContext comment
4b00d36 
Signed-off-by: Peter Alfonsi <[email protected]>
@peteralfonsi peteralfonsi requested a review from a team as a code owner May 19, 2025 19:45
@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for feed2e8: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

spotless apply
c273159 
Signed-off-by: Peter Alfonsi <[email protected]>
@github-actions GitHub Actions
Copy link
Contributor

❕ Gradle check result for c273159: UNSTABLE

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

@opensearch-ci-bot opensearch-ci-bot mentioned this pull request May 20, 2025
Open
@opensearch-trigger-bot opensearch-trigger-bot bot removed the stalled Issues that have stalled label May 20, 2025
@peteralfonsi
Copy link
Contributor Author

Hey @msfroh , any further comments on this one?

@github-actions GitHub Actions
Copy link
Contributor

❌ Gradle check result for c4d29f2: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@peteralfonsi
Copy link
Contributor Author

Flaky test: #18302

rerun gradle
58d9a45 
Signed-off-by: Peter Alfonsi <[email protected]>
@github-actions GitHub Actions
Copy link
Contributor

✅ Gradle check result for 58d9a45: SUCCESS

@opensearch-ci-bot opensearch-ci-bot mentioned this pull request May 27, 2025
Open
@peteralfonsi
Copy link
Contributor Author

Hey @msfroh , just bumping on this

@msfroh msfroh merged commit a6eb368 into opensearch-project:main Jun 4, 2025
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msfroh msfroh msfroh approved these changes

@anasalkouz anasalkouz Awaiting requested review from anasalkouz anasalkouz is a code owner

@andrross andrross Awaiting requested review from andrross andrross is a code owner

@ashking94 ashking94 Awaiting requested review from ashking94 ashking94 is a code owner

@Bukhtawar Bukhtawar Awaiting requested review from Bukhtawar Bukhtawar is a code owner

@CEHENKLE CEHENKLE Awaiting requested review from CEHENKLE CEHENKLE is a code owner

@cwperks cwperks Awaiting requested review from cwperks cwperks is a code owner

@dblock dblock Awaiting requested review from dblock

@dbwiddis dbwiddis Awaiting requested review from dbwiddis dbwiddis is a code owner

@gbbafna gbbafna Awaiting requested review from gbbafna gbbafna is a code owner

@jed326 jed326 Awaiting requested review from jed326 jed326 is a code owner

@kotwanikunal kotwanikunal Awaiting requested review from kotwanikunal kotwanikunal is a code owner

@mch2 mch2 Awaiting requested review from mch2 mch2 is a code owner

@nknize nknize Awaiting requested review from nknize

@owaiskazi19 owaiskazi19 Awaiting requested review from owaiskazi19 owaiskazi19 is a code owner

@reta reta Awaiting requested review from reta reta is a code owner

@Rishikesh1159 Rishikesh1159 Awaiting requested review from Rishikesh1159 Rishikesh1159 is a code owner

@sachinpkale sachinpkale Awaiting requested review from sachinpkale sachinpkale is a code owner

@saratvemulapalli saratvemulapalli Awaiting requested review from saratvemulapalli saratvemulapalli is a code owner

@shwetathareja shwetathareja Awaiting requested review from shwetathareja shwetathareja is a code owner

@sohami sohami Awaiting requested review from sohami sohami is a code owner

@VachaShah VachaShah Awaiting requested review from VachaShah VachaShah is a code owner

@bugmakerrrrrr bugmakerrrrrr Awaiting requested review from bugmakerrrrrr

@jainankitk jainankitk Awaiting requested review from jainankitk

@linuxpi linuxpi Awaiting requested review from linuxpi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@peteralfonsi @rishabhmaurya @msfroh